-
Notifications
You must be signed in to change notification settings - Fork 654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce some policy constants from Bitcoin Core #584
Conversation
dd9e837
to
4a3c771
Compare
Hmm I'm not sure how I feel about standardness constants in here because they depend on Bitcoin Core versions, so they might get outdated. Perhaps we can have a |
There are effectively multiple different types of standardness rules - there are those that depend on Bitcoin Core versions and are likely to be relaxed as a part of a future soft-fork, and those which are effectively anti-DoS rules and are unlikely to be made more strict (though may be relaxed with algorithmic improvements). For better or for worse, things like the max standard weight and dust limits are now a part of the lightning trust model, which secures a nontrivial amount of funds. Changing them to be more strict in the future would imply a potential funds loss vulnerability in lightning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably outside of the scope of this PR we should try to add some consistentcy into which constants from Bitcoin Core we are moving to rust bitcoin and where they should reside.
For now, I have nothing against adding a single constant (it is better than none), but at least from here https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/policy/policy.h#L19-L54 I see a set of constants which can be added alongside MAX_STANDARD_TX_WEIGHT
:
DEFAULT_BLOCK_MAX_WEIGHT
DEFAULT_BLOCK_MIN_TX_FEE
MIN_STANDARD_TX_NONWITNESS_SIZE
I agree, i'll spot the constants i find appropriate and will include them here. Would you conceptually accept this @apoelstra ? |
For the record, I like the idea of having a separate module for standardness constants with a disclaimer as proposed by @stevenroose. Iiuc that would also be a good place for the constants introduced in #566 @TheBlueMatt? |
concept ACK having this value in here. I agree that "in theory" it could change but in practice nobody has ever proposed to change it, and in future Taproot will make massive transactions even less useful. |
Added some constants that i found sensible, leaving the Script-related ones in Renamed Overall i tried to use the same units as
I don't think that it's sensible to add |
4a3c771
to
9995ce3
Compare
9995ce3
to
05a813a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked against constant values in Bitcoin Core. General utACK with few nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 05a813a
Left some minor nits and questions. But nothing blocking if you want to go ahead and merge.
05a813a
to
7d91d9c
Compare
This introduces some constants defined by Bitcoin Core which as a consequence define some network rules in a new 'policy' module. Only some were picked, which are very unlikely to change. Nonetheless a Warning has been put in the module documentation. Script-level constants are left into rust-miniscript where they are already defined (src/miniscript/limits.rs).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
It's very useful to Bitcoin applications, and especially "L2" ones, to effectively compute feerates. Currently (and this is very unlikely to change) bitcoind nodes compute the virtual size as a rounded-up division of the size in witness units by 4, with a penalty for transactions that are essentially >5% full of sigops. Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
7d91d9c
to
7345aa6
Compare
Added two more constants that i forgot and are useful to contract and wallet developers: the default minimum relay feerate and the default mempool expiration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 7345aa6
Test log:
May 21 01:13:36.988 INFO testinator: Installing rust toolchain 'nightly'
May 21 01:14:05.342 INFO testinator: Installing rust toolchain 'stable'
May 21 01:15:31.062 INFO testinator: Installing rust toolchain '1.29.0'
May 21 01:15:32.248 INFO testinator: Preparing environment for rust 1.29.0 tests (8 configurations)
May 21 01:15:32.249 INFO testinator: Preparing environment for rust stable tests (8 configurations)
May 21 01:15:32.249 INFO testinator: Preparing environment for rust nightly tests (9 configurations)
May 21 01:19:55.811 INFO testinator: Running rust 1.29.0 tests in /tmp/rust-bitcoin-1.29.0.nsz3Ox9Bjc22/rust-bitcoin
May 21 01:19:55.812 DEBUG testinator: Generating lock file with rust=1.29.0
May 21 01:19:55.816 INFO testinator: Running rust nightly tests in /tmp/rust-bitcoin-nightly.3qsFJJiFwmF2/rust-bitcoin
May 21 01:19:55.819 INFO testinator: Running rust stable tests in /tmp/rust-bitcoin-stable.aSpzKLNvwhxi/rust-bitcoin
May 21 01:20:12.659 DEBUG testinator: Pinning cc to 1.0.41
May 21 01:20:12.915 DEBUG testinator: Pinning serde to 1.0.98
May 21 01:20:14.681 DEBUG testinator: Pinning serde_derive to 1.0.98
May 21 01:20:15.010 DEBUG testinator: Pinning byteorder to 1.3.4
May 21 01:21:14.712 INFO testinator: Test rust=nightly, features=[secp-recovery] succeeded!
May 21 01:21:16.782 INFO testinator: Test rust=stable, features=[secp-recovery] succeeded!
May 21 01:22:11.653 INFO testinator: Test rust=stable, features=[use-serde] succeeded!
May 21 01:22:17.784 INFO testinator: Test rust=nightly, features=[use-serde] succeeded!
May 21 01:22:18.260 INFO testinator: Test rust=1.29.0, features=[secp-recovery] succeeded!
May 21 01:22:41.497 INFO testinator: Test rust=nightly, features=[base64] succeeded!
May 21 01:22:42.098 INFO testinator: Test rust=stable, features=[base64] succeeded!
May 21 01:23:05.622 INFO testinator: Test rust=nightly, features=[secp-lowmemory] succeeded!
May 21 01:23:07.126 INFO testinator: Test rust=stable, features=[secp-lowmemory] succeeded!
May 21 01:23:28.823 INFO testinator: Test rust=nightly, features=[rand] succeeded!
May 21 01:23:31.392 INFO testinator: Test rust=stable, features=[rand] succeeded!
May 21 01:23:52.468 INFO testinator: Test rust=nightly, features=[unstable] succeeded!
May 21 01:24:12.885 INFO testinator: Test rust=stable, features=[bitcoinconsensus] succeeded!
May 21 01:24:14.254 INFO testinator: Test rust=1.29.0, features=[use-serde] succeeded!
May 21 01:24:43.747 INFO testinator: Test rust=stable, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
May 21 01:24:55.391 INFO testinator: Test rust=nightly, features=[bitcoinconsensus] succeeded!
May 21 01:25:19.925 INFO testinator: Test rust=stable, features=[] succeeded!
May 21 01:25:21.996 INFO testinator: Test rust=1.29.0, features=[base64] succeeded!
May 21 01:25:30.077 INFO testinator: Test rust=nightly, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,unstable,bitcoinconsensus] succeeded!
May 21 01:26:01.512 INFO testinator: Test rust=nightly, features=[] succeeded!
May 21 01:26:12.570 INFO testinator: Test rust=1.29.0, features=[secp-lowmemory] succeeded!
May 21 01:26:57.964 INFO testinator: Test rust=1.29.0, features=[rand] succeeded!
May 21 01:27:45.987 INFO testinator: Test rust=1.29.0, features=[bitcoinconsensus] succeeded!
May 21 01:28:38.155 INFO testinator: Test rust=1.29.0, features=[secp-recovery,use-serde,base64,secp-lowmemory,rand,bitcoinconsensus] succeeded!
May 21 01:29:25.015 INFO testinator: Test rust=1.29.0, features=[] succeeded!
May 21 01:29:27.461 INFO testinator: Fuzzing deserialize_script
May 21 01:30:43.060 INFO testinator: Successfully fuzzed deserialize_script
May 21 01:30:43.060 INFO testinator: Fuzzing uint128_fuzz
May 21 01:31:44.154 INFO testinator: Successfully fuzzed uint128_fuzz
May 21 01:31:44.154 INFO testinator: Fuzzing deserialize_amount
May 21 01:32:45.117 INFO testinator: Successfully fuzzed deserialize_amount
May 21 01:32:45.117 INFO testinator: Fuzzing deserialize_transaction
May 21 01:33:47.297 INFO testinator: Successfully fuzzed deserialize_transaction
May 21 01:33:47.297 INFO testinator: Fuzzing deser_net_msg
May 21 01:34:50.147 INFO testinator: Successfully fuzzed deser_net_msg
May 21 01:34:50.147 INFO testinator: Fuzzing deserialize_address
May 21 01:35:51.105 INFO testinator: Successfully fuzzed deserialize_address
May 21 01:35:51.105 INFO testinator: Fuzzing deserialize_block
May 21 01:36:53.206 INFO testinator: Successfully fuzzed deserialize_block
May 21 01:36:53.206 INFO testinator: Fuzzing outpoint_string
May 21 01:37:55.110 INFO testinator: Successfully fuzzed outpoint_string
May 21 01:37:55.110 INFO testinator: Fuzzing deserialize_psbt
May 21 01:38:58.117 INFO testinator: Successfully fuzzed deserialize_psbt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK, re-checked against Bitcoin Core values. Two nits for comments which can be ignored. If not, pls add them as a separate commit so there will be no need to re-check constant values against Bitcoin Core
//! Policy | ||
//! | ||
//! This module exposes some constants and functions used in the reference implementation and which | ||
//! as a consequence defines some network rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT typo
//! as a consequence defines some network rules. | |
//! as a consequence define some network rules. |
/// mempools. | ||
pub const DEFAULT_MEMPOOL_EXPIRY: u32 = 336; | ||
|
||
/// The virtual transaction size, as computed by default by bitcoind node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit (this is more clear description):
/// The virtual transaction size, as computed by default by bitcoind node. | |
/// The virtual transaction size, as computed by Bitcoin Core `GetVirtualTransactionSize` using default bytes per sigop value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
3 ACKs, CI+1 local test run, minor API change: merged |
Not sure about the place since all other constants are consensus, but it seemed awkward to have it eg in thetransaction
module while there is aconstants
one 🤷EDIT: this now introduces a minimal set of policy stuff from
bitcoind
in a newpolicy
module.